Throttled progress/cancel callback for load_obj_buf - #79
Conversation
LoadOptions gains progress_callback: Option<LoadProgressCallback> (Arc<dyn Fn(&LoadProgress) -> ControlFlow<()> + Send + Sync>, an options field rather than a second _with_progress function), invoked every 1000 lines during load_obj_buf's parse loop. Returning ControlFlow::Break stops the load and returns the new LoadError::Cancelled. No behavior change when progress_callback is None (the default): verified by a test comparing output with and without a no-op callback. load_obj_buf_async is untouched.
| } | ||
|
|
||
| if let Some(callback) = &load_options.progress_callback { | ||
| if lines_read.is_multiple_of(PROGRESS_REPORT_INTERVAL) { |
There was a problem hiding this comment.
It seems like is_multiple_of is a relatively recent Rust function https://doc.rust-lang.org/std/primitive.u64.html#method.is_multiple_of , can we instead just use PROGRESS_REPORT_INTERVAL > 0 && lines_read % PROGRESS_REPORT_INTERVAL == 0 ? to guard against prog report interval being 0 ? It looks like that's the main difference of not using % directly, the is_multiple_of doesn't panic of the rhs is 0
There was a problem hiding this comment.
Swapping to the % form failed clippy's own manual_is_multiple_of lint under -D warnings (the exact check in this PR's own test plan).
Since PROGRESS_REPORT_INTERVAL is a fixed const = 1000 it can never actually be 0 either way, and the method has been stable since 1.87 with nothing here pinning an older toolchain, so I kept is_multiple_of.
Happy to add #[allow(clippy::manual_is_multiple_of)] and go back to the explicit guard if you'd still rather have that for other reasons.
| return Err(LoadError::Cancelled); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we always fire the callback on completion?
There was a problem hiding this comment.
Added an unconditional call after the loop with the true final lines_read/bytes_read, so a progress UI driven off this callback reaches 100% instead of inferring "done" from the Ok return (lines_read only lands on a PROGRESS_REPORT_INTERVAL boundary by chance).
I made ControlFlow::Break on that final call a no-op.
The parse has fully succeeded by then, so there's nothing left to cancel, and discarding a complete result over a stray Break on the terminal ping seemed like the wrong default.
Let me know if you'd rather it be honored uniformly.
Covered by two new tests: test_progress_callback_fires_once_more_on_completion_with_the_true_final_count and test_progress_callback_completion_call_cannot_cancel_an_already_finished_load.
lines_read only lands on a PROGRESS_REPORT_INTERVAL boundary by chance, so a caller driving a progress bar purely off the callback could get stuck short of 100% even though the load succeeded. ControlFlow::Break on this final call is not honored -- the parse has already fully succeeded by then, so there is nothing left to cancel.
check_fmt uses dtolnay/rust-toolchain@nightly, unpinned. Nightly rustfmt's comment-wrap heuristic changed between this PR's original commit (green on 2026-08-25) and now, so CI started flagging pre-existing comment wrapping this PR never touched. Comment rewrap only, no functional change.
2 participants
Summary
LoadOptionsgainsprogress_callback: Option<LoadProgressCallback>(Arc<dyn Fn(&LoadProgress) -> ControlFlow<()> + Send + Sync>), an options field rather than a second_with_progressfunction, so a caller can observe throttled progress and cooperatively cancel a load of a large OBJ buffer.load_obj_buf's parse loop. ReturningControlFlow::Breakstops the load and returns the newLoadError::Cancelled.progress_callbackisNone(the default): verified by a test comparing output with and without a no-op callback.load_obj_buf_async(thefutures/tokiovariants) is intentionally untouched — that's a materially different code path deserving its own design pass.LoadOptionsloses itsCopyderive (Arcisn'tCopy); every existing call site in this crate already passes&LoadOptions, so nothing else needed to change.Motivated by a real use case: an interactive mesh-editing tool importing large OBJ files needed a way to show progress and let a user cancel a slow import without the caller being able to observe anything until the whole call returned.
Test plan
cargo test --all-features: 21 lib tests + 5 doctests pass, including 3 new tests (test_progress_callback_noop_matches_no_callback,test_progress_callback_cancels_load,test_progress_callback_is_throttled)cargo fmt --all -- --check: cleancargo clippy --all-targets --all-features -- -D warnings: clean (exit 0)